-
Notifications
You must be signed in to change notification settings - Fork 69
vhost-device-sound: Add GStreamer audio backend support #876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
the build is failing because
|
right, I just did it at rust-vmm/rust-vmm-ci#190 |
@nicholasdezai we just merged #881 that should bring @MatiasVara changes to add GStreamer deps to our CI container, please rebase this PR. |
I submitted a PR to rust-vmm-container to add the missing GStreamer dependencies |
Please rebase this PR on |
121daab
to
b436032
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for working on this and preparing the container for the CI!
I left some comments, in general I'd avoid unwrap()/expect()
. I mean t's fine for the lock or something that should not happen, but for example you have in some places .ok_or(Error::StreamWithIdNotFound(stream_id))?
and in others .expect("Can not find stream in stream_in.");
. Why? (I didn't look the entire code, so maybe there is a reason).
1e7e0fa
to
859b44e
Compare
I have improved the error handling in the new commit. I hope the error handling makes more sense now |
I have tried both playback and recording. Both operations seem working. |
@dorindabassey @epilys PTAL |
PR LGTM last time I checked, but I’ll do a more thorough review later(tomorrow). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@nicholasdezai please in the meantime rebase this on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @nicholasdezai for the work on this PR, I added just a few comments.
Hi @nicholasdezai , I tested this on Fedora42 with |
you also need to update the readme with the new backend
|
I’m testing with Debian 13 (trixie), kernel 6.15.5. |
I just tried with qemu from master(29b77c1a2db2), Linux v6.16-rc1, and GStreamer 1.24.0, and it seems working. |
I used QEMU from master(517e9b4862cc), gstreamer 1.24.0, kernel 6.16.9, f42, also tried debian-13-nocloud-amd64.qcow2 image with gstreamer 1.24.0 i got no audio, but after updating to 1.24.13 it works with the debian image, so probably not an issue with the backend. |
Add gstreamer backend. Add gstreamer-related crates used in the vhost-device-sound module This affects only the vhost-device-sound module, and updates the following dependencies: - dependency-name: gstreamer dependency-version: 0.24.2 - dependency-name: gstreamer-app dependency-version: 0.24.2 - dependency-name: gstreamer-audio dependency-version: 0.24.2 Signed-off-by: nicholasdezai <[email protected]>
Summary of the PR
This change adds GStreamer audio backend support.
Since the Rust bindings for GStreamer rely on the underlying C libraries, this change introduces additional build dependencies. On Debian/Ubuntu systems, at least libgstreamer1.0-dev and libgstreamer-plugins-base1.0-dev are required.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.